F 1483 : Fix SHA-1 prefix match overwriting SHA-256/384/512 output#216
F 1483 : Fix SHA-1 prefix match overwriting SHA-256/384/512 output#216JacobBarthelmeh merged 3 commits intowolfSSL:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an algorithm-selection bug in the CLI hash command where SHA-1 prefix matching could incorrectly override SHA-256/384/512 digest output, and tightens algorithm validation to require exact matches.
Changes:
- Guard SHA-1 selection in
wolfCLU_hash/wolfCLU_hashSetupwithXSTRLEN(alg) == 3so only"sha"maps to SHA-1. - Change hash algorithm validation to exact string matching (
XSTRCMP) to avoid prefix-based acceptance. - Broaden OCSP interop test error-message matching; update
clu_x509_sign.chash-type switch version gating / case handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/hash/clu_hash.c |
Prevents SHA-1 branch from matching sha256/sha384/sha512 by requiring exact "sha". |
src/hash/clu_hash_setup.c |
Requires exact algorithm match during argument validation and ensures SHA-1 size only applies to "sha". |
tests/ocsp/ocsp-interop-test.sh |
Accepts additional common “file not found” phrasing in Test 6 logs. |
src/x509/clu_x509_sign.c |
Adjusts conditional compilation for unsupported hash-type cases in certificate signing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
miyazakh
force-pushed
the
f-1483_sha1prefix
branch
from
e43bb6a to
1a11f72
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
miyazakh
force-pushed
the
f-1483_sha1prefix
branch
from
1a11f72 to
e1ea92c
Compare
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [Medium] Remaining XSTRNCMP calls could be made consistent with the fix —
src/hash/clu_hash.c:104 - [Low] The fix is correct but the if-chain ordering is fragile —
src/hash/clu_hash.c:108-127
Review generated by Skoll via openclaw
miyazakh
force-pushed
the
f-1483_sha1prefix
branch
from
e1ea92c to
77d7867
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/hash/clu_hash.c:135
- The Blake2b error handling returns immediately on non-zero
ret, which bypasses the common cleanup path (zeroing + freeinginput/output). This leaks memory and can leave sensitive data in heap pages on error. Prefer settingretand falling through to the existing cleanup/return logic (or add a sharedcleanup:label) instead of early returns here.
if (ret == WOLFCLU_SUCCESS && XSTRCMP(alg, "blake2b") == 0) {
ret = wc_InitBlake2b(&hash, size);
if (ret != 0) return ret;
ret = wc_Blake2bUpdate(&hash, input, inputSz);
if (ret != 0) return ret;
ret = wc_Blake2bFinal(&hash, output, size);
if (ret != 0) return ret;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
XSTRNCMP(alg, "sha", 3)inwolfCLU_hashandwolfCLU_hashSetupmatched "sha256", "sha384", and "sha512", causing SHA-1 to overwrite
the correct digest and size
XSTRLEN(alg) == 3guard to the SHA-1 branch inclu_hash.candclu_hash_setup.calgCheckloop inclu_hash_setup.cto useXSTRCMPforexact matching instead of prefix matching
Depend on : #211 (Fixed)
Depend on : #219